Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Interval #441

Open
wants to merge 2 commits into
base: 3.x
Choose a base branch
from
Open

Add Interval #441

wants to merge 2 commits into from

Conversation

trowski
Copy link
Member

@trowski trowski commented Jul 24, 2024

This adds a utility class Interval which controls a repeating timer, automatically cancelling the timer when the object is destroyed.

The main use-case of this object is to store an instance of Interval as an object property. When the holding object is destroyed, the repeating timer will be cancelled. This avoids a bit of boilerplate in the class, such as needing to declare a destructor to manually cancel the event loop callback.

The value here isn't necessarily huge, but this is a pattern we frequently use for classes with self-cleanup mechanisms such as LocalCache.

src/Interval.php Outdated Show resolved Hide resolved
src/Interval.php Outdated
Comment on lines 30 to 33
public function __destruct()
{
EventLoop::cancel($this->watcher);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't there also be a manual cancel method?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it makes more sense for the object to represent a valid repeating timer. If we allow external cancellation, then the object needs to expose an isCancelled() method so the state isn't hidden.

@xpader
Copy link
Contributor

xpader commented Jul 25, 2024

It's just a auto destruct for EventLoop::repeat()??
I think it's not very necessary for a base library.

@trowski
Copy link
Member Author

trowski commented Aug 4, 2024

How about moving this to amphp/sync? That library has sort of become a utility library.

@kelunik
Copy link
Member

kelunik commented Aug 5, 2024

I'd rather keep this here, as it's very basic, no?

@trowski
Copy link
Member Author

trowski commented Aug 5, 2024

@kelunik Yeah, I can't see this needing changing, so seems basic enough to go here. Should we add this then?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants